Add ListBurns RPC
#1178
Add ListBurns RPC
#1178
Conversation
GeorgeTsagk
force-pushed
the
track-burns
branch
from
5c69a1e to
8037aff
Compare
Pull Request Test Coverage Report for Build 12038238720Details
💛 - Coveralls |
GeorgeTsagk
force-pushed
the
track-burns
branch
from
8037aff to
dcb3eee
Compare
GeorgeTsagk
force-pushed
the
track-burns
branch
from
dcb3eee to
a1ddf99
Compare
There was a problem hiding this comment.
Looks pretty good. I wonder if we should add a Golang based "migration" of sort that queries existing assets in the DB, checks if they're burns (cannot be done on SQL level alone) and then inserts them into the new table retroactively?
Perhaps we could add a mechanism that detects if any DB level migrations were executed. And if they were, we can assume an update happened and could run these Golang-level checks (which would need to be idempotent).
GeorgeTsagk
force-pushed
the
track-burns
branch
3 times, most recently
from
7f7febe to
ba5db4b
Compare
rpcserver.go
Outdated
| @@ -3312,12 +3314,56 @@ func (r *rpcServer) BurnAsset(ctx context.Context, | |||
| } | |||
| } | |||
|
|
|||
| // At this point everything completed correctly, so we log this burn in | |||
There was a problem hiding this comment.
Hmm, perhaps we should do this further in the pipeline? So in the same db transaction that we insert the transfer.
Otherwise, if we crash here, then the burn is never inserted in the db, and we exit in an inconsistent state.
There was a problem hiding this comment.
that seems to happen within the state machine of the chain porter, deep in the r.cfg.ChainPorter.RequestShipment step above
threading through all the params needed for the InsertBurn call seems like a bad way to go
There was a problem hiding this comment.
threading through all the params needed for the InsertBurn call seems like a bad way to go
Can you think of an alternative?
We want this visibility to be air tight. If it's possible to just not record a burn all together, then that's problematic.
Perhaps we need a hybrid migration route where we scan the db to insert the new burn metadata into transfers, then can rely on that information being specified in a transfer?
Can you explain the issue you see with adding this meta data at the transfers level to it can be inserted when we go to log the parcel?
There was a problem hiding this comment.
Seems like most of the burn info is available in the Parcel itself, so with the latest push I"m inserting the Burn in the same transaction as the transfer.
Only needed to add the note string argument as an extra to some interfaces, but it's minimal
GeorgeTsagk
force-pushed
the
track-burns
branch
from
ba5db4b to
98816be
Compare
GeorgeTsagk
force-pushed
the
track-burns
branch
2 times, most recently
from
765a010 to
b9334f1
Compare
Just saw this comment, and thought this might come in handy: lightningnetwork/lnd@fba2b5d (Second commit from the WIP PR lightningnetwork/lnd#8831) |
GeorgeTsagk
force-pushed
the
track-burns
branch
from
b9334f1 to
4a97367
Compare
GeorgeTsagk
force-pushed
the
track-burns
branch
from
4a97367 to
8958968
Compare
GeorgeTsagk
force-pushed
the
track-burns
branch
2 times, most recently
from
8777a20 to
fdcd795
Compare
|
rebased, waiting CI then merging |
GeorgeTsagk
force-pushed
the
track-burns
branch
from
fdcd795 to
e9364e0
Compare
Description
We're currently able to provably burn assets with tapd but we have no (easy) way of retrieving burn related historical data. This PR adds a DB table and exposes some new RPC methods/parameters to help keep track of burns.
When completing an asset burn, we add an entry to the new burns table. We also add a new
ListBurnsmethod which returns a list of all burns that pass the user-provided filters (asset id, group id, anchor txid)Closes #1095